[clang-tidy] Avoid expensive AST traversal in RedundantTypenameCheck #170540
Conversation
In Fuchsia, we have several files that take over 2 hours for this check to run, where as it only takes 8 seconds to finish without the RedundantTypenameCheck. We can avoid this exponential behavior by limiting the use of hasAncestor to typeLocs for the types that are actually used in the checking logic. From the wall time for the check with --enable-profile goes from 6724 seconds (about 2 hours) to down to a reasonable 0.1753 seconds.
|
@llvm/pr-subscribers-clang-tools-extra Author: Paul Kirth (ilovepi) ChangesIn Fuchsia, we have several files that take over 2 hours for this check to run, where as it only takes 8 seconds to finish without the RedundantTypenameCheck. We can avoid this exponential behavior by limiting the use of hasAncestor to typeLocs for the types that are actually used in the checking logic. With this patch, the wall time for the check with --enable-profile goes from 6724 seconds (about 2 hours) to down to a reasonable 0.1753 seconds. Full diff: https://github.com/llvm/llvm-project/pull/170540.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
index 5f2519ce9d5c3..f8e576e2a14d7 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
@@ -18,9 +18,13 @@ using namespace clang::ast_matchers;
namespace clang::tidy::readability {
void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(typeLoc(unless(hasAncestor(decl(isInstantiated()))))
- .bind("nonDependentTypeLoc"),
- this);
+ Finder->addMatcher(
+ typeLoc(loc(TypeMatcher(anyOf(typedefType(), tagType(),
+ deducedTemplateSpecializationType(),
+ templateSpecializationType()))),
+ unless(hasAncestor(decl(isInstantiated()))))
+ .bind("nonDependentTypeLoc"),
+ this);
if (!getLangOpts().CPlusPlus20)
return;
|
|
@llvm/pr-subscribers-clang-tidy Author: Paul Kirth (ilovepi) ChangesIn Fuchsia, we have several files that take over 2 hours for this check to run, where as it only takes 8 seconds to finish without the RedundantTypenameCheck. We can avoid this exponential behavior by limiting the use of hasAncestor to typeLocs for the types that are actually used in the checking logic. With this patch, the wall time for the check with --enable-profile goes from 6724 seconds (about 2 hours) to down to a reasonable 0.1753 seconds. Full diff: https://github.com/llvm/llvm-project/pull/170540.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
index 5f2519ce9d5c3..f8e576e2a14d7 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
@@ -18,9 +18,13 @@ using namespace clang::ast_matchers;
namespace clang::tidy::readability {
void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(typeLoc(unless(hasAncestor(decl(isInstantiated()))))
- .bind("nonDependentTypeLoc"),
- this);
+ Finder->addMatcher(
+ typeLoc(loc(TypeMatcher(anyOf(typedefType(), tagType(),
+ deducedTemplateSpecializationType(),
+ templateSpecializationType()))),
+ unless(hasAncestor(decl(isInstantiated()))))
+ .bind("nonDependentTypeLoc"),
+ this);
if (!getLangOpts().CPlusPlus20)
return;
|
| Finder->addMatcher( | ||
| typeLoc(loc(TypeMatcher(anyOf(typedefType(), tagType(), | ||
| deducedTemplateSpecializationType(), | ||
| templateSpecializationType()))), | ||
| unless(hasAncestor(decl(isInstantiated())))) | ||
| .bind("nonDependentTypeLoc"), | ||
| this); |
There was a problem hiding this comment.
Can't we skip the hasAncestor part (I'd keep the anyOf you added anyways since we are producing less matches that way, which also improves perf)
typeLoc(loc(qualType(unless(dependentNameType())))?
Then we are not checking a type that is dependent, which eliminates the need for the hasAncestor and also the
if (NonDependentTypeLoc->getType()->isDependentType())
return SourceLocation();later
There was a problem hiding this comment.
It seems that we can remove unless(...). The traverse mode of this checker is TK_IgnoreUnlessSpelledInSource, so decl(isInstantiated()) doesn't matches anything IIUC.
There was a problem hiding this comment.
Those are great suggestions. Thanks 😃
I’ll try to clean those up and see how our test case works after.
There was a problem hiding this comment.
It seems that we can remove unless(...). The traverse mode of this checker is TK_IgnoreUnlessSpelledInSource, so decl(isInstantiated()) doesn't matches anything IIUC.
I originally added the unless to make this test case pass:
Without the
unless, we get a false positive; the traversal seems to descend into the instantiation despite the traversal mode being TK_IgnoreUnlessSpelledInSource. Experimenting just now, I tried setting the traversal mode on the matcher explicitly:
Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource, typeLoc()
.bind("nonDependentTypeLoc")),
this);And now it does work! Is there maybe an issue causing the traversal mode to not get propagated?
There was a problem hiding this comment.
Ah, bingo. There are a bunch of addMatcher overloads. The ones for DeclarationMatcher and StatementMatcher respect getCheckTraversalKind:
llvm-project/clang/lib/ASTMatchers/ASTMatchFinder.cpp
Lines 1670 to 1680 in 7220268
And all the rest (including the one we're calling) just... don't?!
llvm-project/clang/lib/ASTMatchers/ASTMatchFinder.cpp
Lines 1712 to 1716 in 7220268
There was a problem hiding this comment.
Well, with the updated addMatcher overloads, the simple check you have above seems to work correctly. I put up #170953 to address the overloads, and then I think this PR can just do the simplified check once that lands.
There was a problem hiding this comment.
The simpler matcher also seems to cut the overhead at runtime down significantly. I'll need to re-run it again, but --enable-profile shows its under ~3 seconds now. I'm a little concerned that file still takes a couple minutes to finish reporting under C++20 and only 8 seconds under C++17, so it could be that I held something wrong in my last evaluation.
| .bind("nonDependentTypeLoc"), | ||
| this); | ||
| Finder->addMatcher( | ||
| typeLoc(loc(TypeMatcher(anyOf(typedefType(), tagType(), |
There was a problem hiding this comment.
Can we remove TypeMatcher?
-typeLoc(loc(TypeMatcher(anyOf(...))))
+typeLoc(loc(anyOf(...)))There was a problem hiding this comment.
loc(anyOf(...)) is ambiguous w/o the TypeMatcher. I didn't see an obvious way to make it work any nicer than this. Happy to try a different spelling if you think it would be better, though.
This was noted in llvm#170540, and seems to simply be an oversight. This patch just add the same logic already used in other addMatcher() implementations that honor the traversal kind.
This was noted in llvm#170540, and seems to simply be an oversight. This patch just add the same logic already used in other addMatcher() implementations that honor the traversal kind.
5 participants
In Fuchsia, we have several files that take over 2 hours for this check to run, where as it only takes 8 seconds to finish without the RedundantTypenameCheck. We can avoid this exponential behavior by limiting the use of hasAncestor to typeLocs for the types that are actually used in the checking logic.
With this patch, the wall time for the check with --enable-profile goes from 6724 seconds (about 2 hours) to down to a reasonable 0.1753 seconds under c++17, and 347 seconds under C++20.